This will require changing this test https://github.com/web-platform-tests/wpt/blob/2da079d04f60ebc4c6091d48d01bff8b4f2c423e/webmessaging/broadcastchannel/basics.html#L90

I'll do it as part of servo/servo#25796

Test update PR: web-platform-tests/wpt#21895

@annevk Do you want to review?

FYI I've manually tested with the below simple test:

    let c1 = new BroadcastChannel('closed');
    let c2 = new BroadcastChannel('closed');
    let c3 = new BroadcastChannel('closed');

    c2.onmessage = console.log;
    c3.onmessage = function() {console.log('ok')};
    c1.postMessage('test');
    c2.close();

And FF and Chrome both only print "ok". Webkit doesn't seem to have broadcast channels...

Since there's no way to "open" a BC this change seems like an improvement to me. There seems to be other state that can change though:

• "a global object that is a Window object whose associated Document is fully active" I guess this might be accounted for due to us not running tasks for documents that are not fully active?
• "a global object that is a WorkerGlobalScope object whose closing flag is false and whose worker is not a suspendable worker" I don't think this is accounted for and probably should be? I guess when when a worker's closing flag is set nothing is processed anymore? But then why check it to begin with?

(I guess the other thing we ought to clarify in theory here is that there's a bunch of cross-process lookup things happening that are rather magical.)

Yes you're right, actually I hadn't noticed those inconsistencies.

I think if the window is not fully-active, the task will indeed not run, although it might run later. If the task is not queued a at all, then it can never run. A "suspendable" worker is somewhat similar to a not "fully-active" window, and actually tied in with it, and a suspendable worker can become "active needed" later.

Re the worker closing flag, I think setting it means ignoring new tasks, but still running tasks that have been enqueued up to then(see https://html.spec.whatwg.org/multipage/l#worker-event-loop).

So moving the channel close flag check to inside step 10 makes sense, since by then you're in a task and can make the decision whether to actually fire the message event or not "sync" based on the current state of the channel.

The other things are actually a bit harder to check. Once you're on step 10, you can't really check them since the task wouldn't run at all in the negative cases. So you'd have to indeed check them before queuing the task, which is quite hard since the earlier step are potentially running in another agent-cluster.

Re the magical part, I can only describe how it ended-up being done in Servo:

So how this was done in Servo actually is that there are two tasks. The way to think about it is that the IPC message is received from "inside an agent-cluster" on a IPC router thread. Then a first task is queued on the relevant event-loop, and that task actually runs Step 7(determining the list of "destinations"), and then queues a task for each destination to run Step 9 and 10.

So in that first task we check the worker "closing" flag(if it's a worker event-loop), and this actually kind of makes sense since if it's closing we shouldn't be queuing more task. We also check the window being fully-active(if it's a window event-loop), which actually doesn't make sense since the task wouldn't be running otherwise.

So it's like step 1 to 6 run in a task on the "sender" event-loop, then step 7 runs in one task on the "receiver" event-loop, queued by the IPC router thread, then step 9 and 10 run on a "one task per channel" basis, queued from the task that ran step 7.

Step 8 is actually done both at the level of "the router that knows of all agent-clusters and sends IPC message to them" (by not sending it to where it came from), and for broadcasting "locally" to channels on the same event-loop its done by the task of the sender(by not queuing a task to fire the message event for the channel that initiated the broadcast).

Note that for each broadcast, we always first do a a "local broadcast", which is essentially step 7 on the same event-loop for the same global, in case there are relevant channels on the same global(could be same event-loop, but actually a bit harder to do so just same global for now), then we send the message over IPC to be broadcasted potentially to other globals(in other agent-clusters, and currently also in the same agent-cluster as where it cam from, but not the same global, obviously the IPC hop could be avoided for "same agent-cluster, different global").

So I'd say the current changes are the "low hanging fruit". The others require probably a broader redefinition that would take the IPC hop into account.

Thanks @gterzian!

Were implementation bugs filed for this? I didn't notice this change until now, but chrome's implementation matched the old behavior rather than the new behavior (and unfortunately the newly failing test was missed by our triage process).

It is my understanding from the WHATWG working mode that this should not have been merged without filing such a bug (the linked WPT changes make it clear that chrome is in fact failing the newly added tests).

The failing test wasn't changed though. So did that have the wrong pass condition before?

Sorry about that, I missed that.